fix(spp_api_v2_change_request): fix 3 bugs and promote to Beta#13
fix(spp_api_v2_change_request): fix 3 bugs and promote to Beta#13
Conversation
Summary of ChangesHello @emjay0921, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the robustness and functionality of the Change Request API. It resolves critical issues related to URL routing for complex references, prevents an Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
- Fix path routing: replace /{reference} with /{p1}/{p2}/{p3} on all 8
reference-based endpoints since CR references (CR/2026/00001) contain
slashes that Starlette splits into separate path segments
- Fix mail.thread AccessError: add mail_create_nolog and tracking_disable
context to service env, preventing flush_all() from triggering
message_partner_ids writes as Public user (uid=3)
- Fix detail serialization: exclude mail.thread fields (message_ids,
message_is_follower, has_message, etc.) from API responses
- Promote development_status from Alpha to Beta
emjay0921
force-pushed
the
fix/spp-api-v2-change-request-bugs
branch
from
ec8ffc4 to
228ac0c
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses three bugs, including path routing for CR references with slashes, an AccessError in the service layer, and the exclusion of mail.thread fields from serialization, promoting the module to Beta status. However, the routing fix highlights a lack of granular access control (IDOR) in the API endpoints, and the update logic for change request details is susceptible to mass assignment due to insufficient input filtering. For improved maintainability, consider refactoring repetitive path parameter handling in routers/change_request.py into a single FastAPI dependency and programmatically identifying mail.thread fields in services/change_request_service.py.
| def _build_reference(p1: str, p2: str, p3: str) -> str: | ||
| """Reconstruct CR reference from path segments (e.g., CR/2026/00001).""" | ||
| return f"{p1}/{p2}/{p3}" |
There was a problem hiding this comment.
This helper function is a good first step, but its repeated use in each of the 8 updated endpoints leads to significant code duplication. A more idiomatic and maintainable approach in FastAPI is to use a dependency.
You can convert this logic into a dependency that extracts the path parameters and provides the reconstructed reference string directly to the endpoint functions. This will centralize the path-parsing logic and make the endpoint signatures much cleaner.
Here's how you could refactor this:
-
Define the dependency function (you can rename
_build_reference):def get_cr_reference_from_path( p1: Annotated[str, Path(description="Reference part 1 (e.g., CR)")], p2: Annotated[str, Path(description="Reference part 2 (e.g., 2026)")], p3: Annotated[str, Path(description="Reference part 3 (e.g., 00001)")], ) -> str: """Dependency to reconstruct CR reference from path segments.""" return f"{p1}/{p2}/{p3}"
-
Use the dependency in your endpoints:
@change_request_router.get("/{p1}/{p2}/{p3}", response_model=ChangeRequestResponse) async def read_change_request( reference: Annotated[str, Depends(get_cr_reference_from_path)], env: Annotated[Environment, Depends(odoo_env)], # ... other dependencies ): # The `reference` variable is now injected by FastAPI. # No need for p1, p2, p3 parameters or the _build_reference call inside. service = ChangeRequestService(env) cr = service.find_by_reference(reference) # ...
Applying this pattern to all affected endpoints will greatly improve the code's readability and reduce redundancy.
| # mail.thread fields | ||
| "message_ids", | ||
| "message_follower_ids", | ||
| "message_partner_ids", | ||
| "message_is_follower", | ||
| "has_message", | ||
| "message_needaction", | ||
| "message_needaction_counter", | ||
| "message_has_error", | ||
| "message_has_error_counter", | ||
| "message_attachment_count", | ||
| "message_has_sms_error", | ||
| "message_main_attachment_id", | ||
| "website_message_ids", | ||
| } |
There was a problem hiding this comment.
Hardcoding the list of mail.thread fields to skip is effective for now, but it can be brittle. If the mail.thread mixin is updated in a future version of Odoo, this list would need to be manually updated to avoid leaking new fields.
A more robust and maintainable approach is to programmatically retrieve the fields from the mail.thread model. This ensures your exclusion list is always synchronized with the mail.thread definition.
Consider this alternative approach:
# In _serialize_detail method
# Get fields from the model
model_fields = detail._fields
# Base fields to skip
skip_fields = {
"id",
"create_uid",
"create_date",
"write_uid",
"write_date",
"__last_update",
"display_name",
"change_request_id",
"registrant_id",
"approval_state",
"is_applied",
}
# Programmatically add all mail.thread fields
mail_thread_fields = self.env["mail.thread"]._fields.keys()
skip_fields.update(mail_thread_fields)
for field_name, field in model_fields.items():
# ... rest of the methodThis makes the code more resilient to changes in the underlying Odoo framework.
1 participant
Why is this change needed?
Three bugs were discovered during curl testing of the Change Request API:
flush_all()triggers mail.threadmessage_partner_idswrites as Public user (uid=3), causing 403 AccessErrormessage_is_follower,has_message, etc.) in API responsesHow was the change implemented?
/{reference}with/{p1}/{p2}/{p3}on all 8 route definitions + added_build_reference()helper to reconstruct the referencemail_create_nolog=Trueandtracking_disable=Truecontext to service env in__init__, preventing flush_all() from triggering mail.thread operationsskip_fieldsset in_serialize_detailNew unit tests
Unit tests executed by the author
Ran
spp_api_v2_change_requesttest suite: 21 tests, 0 failed, 0 errorsHow to test manually
POST /api/v2/spp/oauth/tokenPOST /api/v2/spp/ChangeRequest— verify successGET /api/v2/spp/ChangeRequest/CR/2026/NNNNN— verify returns data (was 404)GET /api/v2/spp/ChangeRequest?status=draft— verify still worksPUT /api/v2/spp/ChangeRequest/CR/2026/NNNNN$submit,$approve,$reject,$request-revision,$apply,$resetmessage_*orhas_messagefieldsRelated links